Fix UAF of a suspended coroutine referenced only via a closure - #1569
Conversation
|
@bnoordhuis Here is the context for the GC-stress PR. When I added it to txiki.js one of my tests started to fail. This is the smallest possible repro. I have an AI generated fix but I'm not super-confident about its correctness here. |
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
I don't know about this approach. Apart from all the other changes, enlarging JSVarRef is undesirable.
It's only possible when the async function's promise is not awaited, right? What about putting all live promises on a linked list and keep them artificially alive during GC?
Pro: also a useful tool for debugging promise leaks on context destruction.
Con: adds a O(n) step to the GC phase.
Another approach/hack: artificially increase the promise's reference count by one when it's created, then decrement it when it's awaited, then'd, etc. In other words, prevent it from getting reclaimed unless it's consumed by something else.
|
Thanks for taking a look! Those sound like good ideas, I'll take a look! |
6469041 to
46a3d0f
Compare
|
@bnoordhuis I did another pass (cleanup pending). The new approach doesn't extend the size of I couldn't get your proposal(s) to work because the bug also affects 2 more case without promises involved: plain generators and the mapped arguments fast path. |
| (e.g. mapped `arguments`) must stay non-coro even though cur_gc_obj is | ||
| later set. Fits existing padding, so JSVarRef does not grow. Cleared | ||
| when the var_ref is detached; the ref is released on detach/free. */ | ||
| uint8_t is_coro; |
There was a problem hiding this comment.
Isn't this implied by !var_ref->detached && var_ref->stack_frame->cur_gc_obj != NULL? If that's the case, it'd be better to remove is_coro and have something like bool js_is_coro(JSVarRef *vr).
There was a problem hiding this comment.
Updated the comment, it can't because it changes over time.
A suspended coroutine (async function, generator or async generator) keeps its activation frame in a heap-allocated GC object. When a closure captures one of the coroutine's locals it holds an *open* var_ref that points into that frame. Open var_refs were not GC objects and were not traced by their holders, so the edge closure -> var_ref -> coroutine frame was invisible to the cycle collector: a coroutine reachable only through such a closure was collected while still live, and resuming it (or running the closure) then touched freed memory. Make an open var_ref that captures a coroutine local a GC object holding a counted reference to the owning coroutine (JSStackFrame.cur_gc_obj) and mark it from every var_ref holder, so the collector sees the edge and keeps the suspended coroutine (and the frame the var_ref points into) alive.
46a3d0f to
c34d173
Compare
saghul
changed the title
2 participants
No description provided.